Add feature to ignore git commits by id #327

philippn · 2022-03-27T08:53:21Z

Hey @mathieucarbou ,

here is the PR, thanks for your consideration.

Kind regards,
Philipp

philippn · 2022-03-27T09:14:49Z

I also rebased it onto master just now

mathieucarbou · 2022-03-27T10:49:35Z

@philippn : thanks a lot!

Could you please also update the README.md in the git module to add a few lines to document this new feature ? There is a usage example that can be updated at least, plus eventually adding a comment line to explain the setting or a line below or above.
You can also update the index.md file in the docs folder to add yourself in the contributor list ;-)

@dbwiddis @hazendaz : PR looks good to me as-is but please add your review if you can since you contribute a lot you might have your word on it too ;-)

dbwiddis

Comments inline.

Also you should add a test to GitLookupTest to verify that it works.

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java

...aven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitPropertiesProvider.java

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java

...aven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitPropertiesProvider.java

license-maven-plugin-git/src/test/java/com/mycila/maven/plugin/license/git/GitLookupTest.java

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java

dbwiddis · 2022-03-27T20:10:46Z

...aven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitPropertiesProvider.java

+          Set<String> commitsToIgnore = Collections.emptySet();
+          if (commitsToIgnoreString != null) {
+            commitsToIgnoreString = commitsToIgnoreString.trim();
+            commitsToIgnore = Arrays.stream(commitsToIgnoreString.split(","))


One other suggestion. Splitting on a comma forces users into a long one-line string here. You could split on a non-hex digit, e.g., [^0-9a-fA-F]+ (and also filter out empty strings after the split) which would allow flexibility such as listing commits on newlines like:

<license.git.commitsToIgnore> 6eb93dcfc014bfd3980c1f1c43f10fd59422107e, ad1e59a6afc5a1a7edbdc8bcf949bb2f8682f074, 70403fdbc424e5d2045389e685f9bb9bd696d5cd </license.git.commitsToIgnore>

By the way, the reason I'm making this suggestion is that I really think this is a cool feature and I'm eager to use it, and somewhat biased by how I want to use it. ;)

I looked into this, I'm a bit vary to use lots of regex magic here as it is not easily testable. It would probably make sense to create a test for GitPropertiesProvider, but that would require factoring out the instantiation of the GitLookup into some kind of factory.

So for the time being, I added a trim operation to the streaming expression, that should basically result in the same behaviour. So technically, I believe your example above should work :-)

I looked into this, I'm a bit vary to use lots of regex magic here as it is not easily testable.

Fair enough. I hit regex101 to play around with my proposal and it took a few iterations to confirm what I expected; and the added complexity of producing empty strings that you then have to filter was undesirable. I like your trim() solution and it (should) work with my use case.

It would probably make sense to create a test for GitPropertiesProvider, but that would require factoring out the instantiation of the GitLookup into some kind of factory.

Yeah. I'm not fond of the existing lazy initialization but haven't had time to think of a better way to do it.

It would probably make sense to create a test for GitPropertiesProvider, but that would require factoring out the instantiation of the GitLookup into some kind of factory.

Yeah. I'm not fond of the existing lazy initialization but haven't had time to think of a better way to do it.

So that is fixed in #339

philippn · 2022-03-27T23:39:14Z

I'm a bit overwhelmed by all this feedback, please bear with me :-)

So in the order of the points being mentioned:

I added a paragraph to the readme.md
I added myself to the contributors, thanks for pointing that out
I added two new tests to GitLookupTest; now here is actually where things start to get interesting. On my machine (Windows, IntelliJ) the tests weren't picked up, neither by IntelliJ itself nor by Maven. Dunno whats going on here but changing line 60 to gitRepoRoot = unzipDestination.resolve("git-test-repo"); solved the issue for me, at least it is then running in IntelliJ.

Regarding the concrete things pointed out by @dbwiddis I will comment on each individually.

Thanks for your feedback, it's much appreciated!

dbwiddis · 2022-03-28T00:04:25Z

I'm a bit overwhelmed by all this feedback, please bear with me :-)

Speaking for project maintainers in general, we are thrilled with contributions from the community and we will happily bear with you as needed. Thanks for contributing and being so gracious with the feedback.

This LGTM at this point. Not sure what's going on with the tests, but if @mathieucarbou wants to merge this push out an rc3 that I can fiddle with real-life testing on my own project I'm happy to do that!

dbwiddis · 2022-03-28T00:16:57Z

@hazendaz JDK 17 / 18 tests are failing with "Not executing Javadoc as the project is not a Java classpath-capable package" ... hope that's something you're already working on. Do we need to rebase this PR?

hazendaz · 2022-03-28T00:53:18Z

are failing with "Not executing Javadoc as the project is not a Java classpath-capable package" ... hope that's something you're already working on. Do we need to rebase this PR?

Had not seen that yet but will take a look at that one. Thanks.

hazendaz · 2022-03-28T00:56:43Z

@dbwiddis javadoc may be unrelated issue. The issue that jump sout at failure is test coverage drops down by 1% on 17/18. Not sure exactly why but I had the same issue when setting jacoco up there. Typically I go with more strict enforcement of coverage but in this case wanted to go with the closest to IDE coverage stats in Eclipse. So to fix that @philippn, its easy go to the 2 submodule poms (license-maven-plugin and license-maven-plugin-git. Just drop the jacoco.minimum coverage number by 1% in both. That should get it to green here.

mathieucarbou

Please wait for #339 to be merged first and rebase onto after.

#339 is merged

mathieucarbou · 2022-03-29T19:15:15Z

@dbwiddis : your merge commit looks weird ;-)
The correct way for this PR is to rebase the original commit on top of master, solve conflicts, and merge it with a merge commit.
Don't know if @philippn has the time for that, or otherwise if not we rebase and force push his commit if his branch his opened, or create a new PR.

mathieucarbou

PR has to be rebased on top of master and conflicts solved as part of the rebase.

dbwiddis · 2022-03-29T19:19:33Z

@dbwiddis : your merge commit looks weird ;-)

Github commandline ain't all it's cracked up to be.

Unfortunately the whole #339 refactor made resolving conflicts nontrivial. Will try again.

dbwiddis · 2022-03-29T20:12:15Z

Don't ask why there are two merge commits. But I've fixed conflicts 4 times now and I think it's correct now. 😁

mathieucarbou · 2022-03-29T20:39:40Z

Don't ask why there are two merge commits. But I've fixed conflicts 4 times now and I think it's correct now. 😁

I'll check. Not normal... gît fetch upstream && gît rebase -i upstream/master then fix conflits during rebase, there should be one commit and no merge commit, then force push.

dbwiddis · 2022-03-29T20:46:55Z

Yeah, I know how it's supposed to work. Between the GitHub CLI and my IDE things didn't work out well the first time.

I successfully merged the second go around but then ended up unable to push back to this branch, so I cherry-picked the merge commit onto this branch and it seemed to work, with the exception of showing two commits.

mathieucarbou · 2022-03-29T20:55:10Z

That is normal that you were not able to push back because rebasing changes the history and commit ID to place it on top of master. You have to force the puah, which will rewrite the branch history for this commit and GitHub will show your icon next to @philippn 's icon

dbwiddis · 2022-03-29T21:33:31Z

Well, in any case, the two commits are identical and the result of all this is correctly working code that can be merged. If someone wants to put in the work to back those commits out and start afresh, feel free. Having done the same changes 4 times, I'm done. 😁

dbwiddis · 2022-03-30T06:44:23Z

Well, that worked just fine at home on my mac. 😏

mathieucarbou · 2022-03-30T07:37:34Z

Well, that worked just fine at home on my mac. smirk

Still not rebased on master if we look at the graph: https://github.com/mathieucarbou/license-maven-plugin/network

mathieucarbou · 2022-03-30T09:30:26Z

Well, that worked just fine at home on my mac. smirk

Still not rebased on master if we look at the graph: https://github.com/mathieucarbou/license-maven-plugin/network

Solved.
The network graph looks better now: https://github.com/mathieucarbou/license-maven-plugin/network

FYI

❯  git fetch --all
❯  git remote-add-gh philippn  # a custom macro I did to add a fork and fetch it
❯  git checkout philippn/features/ignore-commits -b ignore-commits
❯  git rebase upstream/master
❯  git push -f philippn HEAD:features/ignore-commits

When doing changes for maven / build / infra stuff, I am less "picky", but when merging code related to enhancement and features I want the commits to be rebased on master and merged with a merge commit to maintain a clean history.

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java

… and cleanup property parsing

mathieucarbou added is:feature New feature in:git MLP Git integration labels Mar 27, 2022

mathieucarbou added this to the 4.2 milestone Mar 27, 2022

mathieucarbou linked an issue Mar 27, 2022 that may be closed by this pull request

Add feature to ignore certain commits #292

Closed

mathieucarbou requested review from mathieucarbou and hazendaz March 27, 2022 10:51

mathieucarbou added the todo Accepted items from the backlog which can be worked on label Mar 27, 2022

dbwiddis suggested changes Mar 27, 2022

View reviewed changes

dbwiddis reviewed Mar 27, 2022

View reviewed changes

dbwiddis mentioned this pull request Mar 27, 2022

Enforce coding style #330

Closed

dbwiddis approved these changes Mar 27, 2022

View reviewed changes

hazendaz mentioned this pull request Mar 28, 2022

[import] Sort the imports to resolve checkstyle warnings #338

Merged

mathieucarbou previously requested changes Mar 28, 2022

View reviewed changes

mathieucarbou mentioned this pull request Mar 28, 2022

Rewrote the PropertiesProvider API to support initialization, close and better multithreading #339

Merged

dbwiddis approved these changes Mar 29, 2022

View reviewed changes

mathieucarbou self-requested a review March 29, 2022 19:17

mathieucarbou requested changes Mar 29, 2022

View reviewed changes

mathieucarbou requested changes Mar 30, 2022

View reviewed changes

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java Outdated Show resolved Hide resolved

mathieucarbou approved these changes Mar 30, 2022

View reviewed changes

philippn and others added 2 commits March 30, 2022 13:04

Add feature to ignore git commits by id

6f6a551

Removed unnecessary failure regarding TZ, filtering out empty strings…

7f8396b

… and cleanup property parsing

mathieucarbou merged commit 2fbfe95 into mathieucarbou:master Mar 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to ignore git commits by id #327

Add feature to ignore git commits by id #327

philippn commented Mar 27, 2022

philippn commented Mar 27, 2022

mathieucarbou commented Mar 27, 2022 •

edited by dbwiddis

Loading

dbwiddis left a comment •

edited

Loading

dbwiddis Mar 27, 2022

dbwiddis Mar 27, 2022

philippn Mar 27, 2022

dbwiddis Mar 27, 2022

mathieucarbou Mar 28, 2022

philippn commented Mar 27, 2022 •

edited

Loading

dbwiddis commented Mar 28, 2022

dbwiddis commented Mar 28, 2022

hazendaz commented Mar 28, 2022

hazendaz commented Mar 28, 2022

mathieucarbou left a comment

mathieucarbou commented Mar 29, 2022

mathieucarbou left a comment

dbwiddis commented Mar 29, 2022

dbwiddis commented Mar 29, 2022

mathieucarbou commented Mar 29, 2022 •

edited

Loading

dbwiddis commented Mar 29, 2022

mathieucarbou commented Mar 29, 2022

dbwiddis commented Mar 29, 2022

dbwiddis commented Mar 30, 2022

mathieucarbou commented Mar 30, 2022

mathieucarbou commented Mar 30, 2022 •

edited

Loading

Add feature to ignore git commits by id #327

Add feature to ignore git commits by id #327

Conversation

philippn commented Mar 27, 2022

philippn commented Mar 27, 2022

mathieucarbou commented Mar 27, 2022 • edited by dbwiddis Loading

dbwiddis left a comment • edited Loading

Choose a reason for hiding this comment

dbwiddis Mar 27, 2022

Choose a reason for hiding this comment

dbwiddis Mar 27, 2022

Choose a reason for hiding this comment

philippn Mar 27, 2022

Choose a reason for hiding this comment

dbwiddis Mar 27, 2022

Choose a reason for hiding this comment

mathieucarbou Mar 28, 2022

Choose a reason for hiding this comment

philippn commented Mar 27, 2022 • edited Loading

dbwiddis commented Mar 28, 2022

dbwiddis commented Mar 28, 2022

hazendaz commented Mar 28, 2022

hazendaz commented Mar 28, 2022

mathieucarbou left a comment

Choose a reason for hiding this comment

mathieucarbou commented Mar 29, 2022

mathieucarbou left a comment

Choose a reason for hiding this comment

dbwiddis commented Mar 29, 2022

dbwiddis commented Mar 29, 2022

mathieucarbou commented Mar 29, 2022 • edited Loading

dbwiddis commented Mar 29, 2022

mathieucarbou commented Mar 29, 2022

dbwiddis commented Mar 29, 2022

dbwiddis commented Mar 30, 2022

mathieucarbou commented Mar 30, 2022

mathieucarbou commented Mar 30, 2022 • edited Loading

mathieucarbou commented Mar 27, 2022 •

edited by dbwiddis

Loading

dbwiddis left a comment •

edited

Loading

philippn commented Mar 27, 2022 •

edited

Loading

mathieucarbou commented Mar 29, 2022 •

edited

Loading

mathieucarbou commented Mar 30, 2022 •

edited

Loading